Skip to content

feat!: regenerate the SDK from the enhanced generator - #131

Open
mridang wants to merge 35 commits into
mainfrom
feat/better-enhanced-sdks
Open

mridang wants to merge 35 commits into
mainfrom
feat/better-enhanced-sdks

Conversation

@mridang

@mridang mridang commented Jun 13, 2026 •

Copy link
Copy Markdown
Collaborator

Regenerates the SDK from the enhanced openapi-generator-plus, with bespoke authenticators ported to the new interfaces and full unit + integration suites passing locally.

Closes #44
Closes #113
Closes #83

@github-actions

github-actions Bot commented Jun 13, 2026 •

Copy link
Copy Markdown
Contributor

Qodana for JVM

1 new problem were found

Inspection name Severity Problems
Nullability and data flow problems 🔶 Warning 1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

@mridang
mridang force-pushed the feat/better-enhanced-sdks branch from d3e4357 to caa8a10 Compare June 13, 2026 13:18
@mridang mridang changed the title feat: regenerate SDK from enhanced generator with ported authenticators feat!: regenerate the SDK from the enhanced generator Jun 13, 2026
@mridang
mridang force-pushed the feat/better-enhanced-sdks branch 11 times, most recently from fa58f11 to 0020a93 Compare June 14, 2026 13:59

@aymenmehri aymenmehri left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a maintainer — just a user of this SDK who went through the diff because it has been sitting for a while. Filtering out the 1377 generated model/ and api/ files leaves ~97 reviewable ones, so this is feedback on those plus the build config. Hope it is useful; happy to be wrong on any of it.

What clearly improves

  • Test coverage goes from 18 to 215 @Test methods. That alone is worth a lot on a client SDK.
  • Generated models shrink roughly 4x — UserServiceAddHumanUserRequest drops from 608 to 147 lines — with serde centralized in ObjectSerializer. This is the outcome #44 was after, even though it landed via a serializer rather than the ZitadelModel base class the issue sketched. Worth saying so explicitly when closing it, since the mechanism differs from what was proposed.
  • The typed exception hierarchy (errors/BadRequestException, UnauthorizedException, NotFoundException, …) is a real ergonomics win over catching ApiException and switching on an int.
  • The redirect hardening in DefaultApiClient is genuinely good: refusing non-HTTP(S) redirect schemes, stripping Authorization on an HTTPS→HTTP downgrade, and refusing to replay a request body across that downgrade. Same for preserving the transport cause via ApiException(String, Throwable).

Blocking, in my view

1. The PR silently requires Java 25

pom.xml sets <release>25</release>, up from 11 on main, and .github/workflows/integration.yml collapses the matrix from [ '11', '17', '21' ] to [ '25' ]. Neither the PR description nor the README mentions this.

Concretely, on Temurin 21.0.11 the build does not start:

[ERROR] error: invalid flag: -Xlint:-dangling-doc-comments
[ERROR] Failed to execute goal ...maven-compiler-plugin:3.14.0:compile

For an application this would be a maintainer's call. For a client library it means every consumer must be on Java 25 to use the SDK at all — <release>25</release> puts class file major version 69 in the published jar. Java 11, 17 and 21 are where nearly all deployed workloads sit, and the compatibility matrix no longer proves the SDK works on any of them.

If the Java 25 syntax in use is what forces this, most of it degrades cheaply: @java.io.Serial is Java 14+ and removable, switch expressions are Java 14+. A <release>17</release> or 21 baseline looks reachable. If Java 25 is a deliberate product decision, it needs to be in the PR title/body, the README, and the release notes, because it is a far bigger breaking change than the API rename that feat! currently signals.

2. Closes #113 does not hold

#113's first task is "Remove usage of any v2beta/ endpoints". The branch still issues 143 calls to v2beta service paths, byte-identical to main:

// src/main/java/com/zitadel/api/BetaOrganizationServiceApi.java
String path = "/zitadel.org.v2beta.OrganizationService/CreateOrganization";

git grep -c v2beta returns the same 143 hits on both branches, and the ten Beta*ServiceApi classes are all still there. Since V5 removes those endpoints, merging this as-is would break the SDK against precisely the release #113 exists to support. This reads like it needs its own PR, and #113 should stay open.

3. Closes #83 is addressed only incidentally

BaseApi.throwApiException builds the message as:

String message = "API returned status code " + code;

So the root cause is still not in the message — "Error 400" merely became "API returned status code 400". The reporter's body content does surface, but only because ApiException.getMessage() was overridden to dump the whole object:

return "ApiException{" + "statusCode=" + statusCode + ", message='" + super.getMessage()
     + "', responseHeaders=" + responseHeaders + ", responseBody='" + responseBody + '\'' + '}';

Two problems with relying on that:

  • It is a toString() wearing getMessage()'s clothes. Every log line and every stack trace header now carries the full header map and full body inline, which is rough on anyone aggregating logs.
  • It inlines every response header into the exception message. That sits awkwardly next to the three commits in this PR that redact secrets in authenticator toString() — the same reasoning applies here, and response headers are not guaranteed to be free of sensitive values.

What #83 actually asked for was "Error 400: invalid OrganizationNameQuery.Name: value length must be between 1 and 200 runes, inclusive" — pull message out of the parsed errorBody in throwApiException and append it, then leave getMessage() alone and keep the dump in toString().

4. Minor: the README keeps a stale requirement

The rewritten README still says "Ensure you have Java 8 or higher installed" (line 39), which was already wrong against <release>11</release> and is very wrong against 25. I opened #136 / #137 against main for that line; whichever lands second should carry the fix, otherwise this PR reintroduces it.

Summary

The transport, error model and test story here are a solid step up, and I would happily use this SDK. The Java 25 baseline is the one I would not merge without an explicit decision, and I think #113 and #83 should stay open rather than be closed by this PR.

Regenerate the client from openapi-generator-plus with modernized
templates, authenticators ported to the new interfaces, and house
tooling aligned to the generator's output.

BREAKING CHANGE: new generated API surface, a raised minimum runtime,
and updated dependencies; not source-compatible with the prior release.
Both were referenced nowhere: Version held a stale hardcoded VERSION constant
and StringUtil reimplemented String.join.
Reorganize .openapi-generator-ignore into the shared section convention and
sort entries alphabetically within each section. No kept paths added or
dropped.
Move getPrivateKeyFromString into WebTokenAuthenticator and buildHostname
into OpenId, NoAuthAuthenticator and PersonalAccessTokenAuthenticator as
private helpers, then delete the shared utils package. Other language SDKs
inline these too.
Drop lefthook from devbox, gitignore devbox.d, and trim the keep-list:
remove the already-gitignored .env and .idea entries and normalize
directory globs from dir/** to dir/.
Regeneration syncs the stale Client references in SKILLS.md to the
generated Zitadel facade.
The CMD points jshell at target/lib/* but mvn install never copied
dependencies there, so the REPL failed with 'target/lib/* not found'.
Add a dependency:copy-dependencies step to the build.
Override toString on token/secret-holding authenticators so secrets and
cached tokens are masked as *** instead of leaking into logs, matching
the Python, PHP and Ruby SDKs.
Regenerated against the generator that now redacts the bearer token in
BearerAuthenticator and ZitadelAccessTokenAuthenticator toString(), so the
masking is template-driven and survives regeneration instead of being
reverted. Keep-list the bespoke AuthenticatorRedactionTest so the prune step
preserves it.
Regenerated against the generator that redacts the bearer token in
BearerAuthenticator and ZitadelAccessTokenAuthenticator toString(). Replace the
aggregate AuthenticatorRedactionTest with one redaction test per bespoke
authenticator, each in its own test file (mirroring the Python, PHP, Ruby and
Node SDKs); keep-list them so the prune step preserves them.
@mridang
mridang force-pushed the feat/better-enhanced-sdks branch from b685a50 to 6aa2c24 Compare September 17, 2026 02:09
The generator lowercased only the first character of a camelCase identifier, so
names beginning with an acronym kept a stray run of capitals. It also selected
deep array comparison for map-valued properties, emitting a Collection call on a
Map so the SDK did not compile.

This regeneration also folds in accumulated transport work the committed tree
was behind on: multipart byte and file handling, authenticator precedence,
gzip error handling and BOM-less UTF-16 decoding. Session model equality now
compares array contents rather than references.

BREAKING CHANGE: oIDCService and sAMLService are renamed to oidcService and samlService.
The generated DefaultApiClient unit test referenced PhotoMetadata, a model that
exists only in the generator's own petstore spec, so the test sources did not
compile against this spec. The fixture is now declared in the test itself.
SKILLS.md leaves the keep-list. It is a generated file, but it sat in the block
reserved for team-owned tooling, so the generator never rewrote it and it froze
against an old version. It documented a petApi property and a Pet model this
package has never had, and told callers to import com.zitadel.models, which is
not the name of the package.

Model equality now compares container contents rather than references, so byte
arrays held in a list or a map compare by value. Signatures are unchanged.
Picks up the generated-documentation fixes. The examples previously described
APIs this package does not have: a RequestContext parameter no generator
emits, accessors under the wrong name, and error handling that could not run
as written.
The twelve generators had drifted into twelve dialects describing the same
SDK. Each sentence now has one wording across every language, varying only in
a token the language genuinely calls by another name.
The package manifest is maintained by hand and already declares Apache-2.0;
recording it in proc.yml keeps the generated metadata in agreement.
Adds the network and timeout errors, drops the one-off error types and
keeps the Zitadel root name through errorPrefix.
OpenID discovery and token requests now run lazily through the injected
ApiClient. Caller mistakes raise the built-in argument or state error, a
rejected token request OAuth2Server, an unusable token response OAuth2Token.
The private-key spec now uses the local stack instead of zitadel.cloud.
Every SDK error now lives in com.zitadel.errors, so the hand-written auth,
tests and specs import from there. Drop the local OAuth2 exception copies now
that the generator always emits them, and let OpenId discovery raise through
ApiException.fromResponse instead of keeping its own status table.

Add the OpenTelemetry SDK test dependencies the regenerated trace-context test
needs.
The repo carried no contributor guidance, so the keep-list, the error
contract and where a fix belongs were only discoverable by reading code.
Errors, client contract and configuration now match the other eleven
languages; the process-wide default configuration is gone.
The generator now records which dev dependencies its tests import, and
this repo keeps its own package manifest, so the two must stay in step.
…anifest

The generator now records which dev dependencies its tests import, and its
linter configs apply to the generated tree instead of being kept locally.
Generated code now passes the SDK toolchain: java line length is off as in
the other SDKs, python prunes unused imports, C# uses one authenticator form.
Drop testcontainers, assertj and junit-jupiter-params (no generated test
uses them) and declare opentelemetry-sdk-trace, which the trace test imports.
This is what dependency:analyze checks; the three unused deps were added in
error and broke the previously-green Lint Dependencies gate.
Add the 3129 auth proxy port, a testcontainers harness, and basic/407/authed proxy tests.
@mridang
mridang force-pushed the feat/better-enhanced-sdks branch from 3ee1993 to 5be7f35 Compare September 28, 2026 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Release SDK for V5 Please include the root cause in the exception message Fixed the messy auto-generated serde logic in the library

2 participants